-
Notifications
You must be signed in to change notification settings - Fork 2
ioctl: delete nvme_zns_report_zones invalid parameters #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Daniel Wagner <[email protected]>
Abstract the file descriptor handle to a more abstract type so it's possible to use different 'backends' to send/receive commands, e.g. via MI instead direct. The name 'link' was choosen because this name is not in use and thus we avoid overloading existing names, such ctrl. The specification is using 'link' to describe the connection on a PCI bus which is kind of matching how the handle is used in the library. It is the means to talk to a controller. Signed-off-by: Daniel Wagner <[email protected]>
Since uring not supported for link type mi. Signed-off-by: Tokunori Ikegami <[email protected]>
There is no user left for the file descriptor in the args structs, thus remove it. Signed-off-by: Daniel Wagner <[email protected]>
When the user provides their own implemention of the passtrhu function, it is necessary to use the file descriptor for the ioctl call. Thus allow to retrieve the file descriptor from the link handle. Signed-off-by: Daniel Wagner <[email protected]>
nvme-cli wants to retrieve the name of the device, add a getter for this. Signed-off-by: Daniel Wagner <[email protected]>
Signed-off-by: Daniel Wagner <[email protected]>
Signed-off-by: Daniel Wagner <[email protected]>
Signed-off-by: Daniel Wagner <[email protected]>
Signed-off-by: Daniel Wagner <[email protected]>
Signed-off-by: Daniel Wagner <[email protected]>
The MI code has the aligment and lenght check which the the direkt APIs are missing. Without these checks the download tests for MI will fail after using the generic APIs. Signed-off-by: Daniel Wagner <[email protected]>
With the introduction of the link handle, it's possible to use the generic nvme API. The transport selection happens on lower levels depending on the link object type. Note, the tests rely on using the ioctl interface and not the io_uring. Signed-off-by: Daniel Wagner <[email protected]>
The v1 design used the POSIX way to return errors, return -1 with errno set. Replace this design by returning directly the error code. That means non trivial function have to return any data via a pointer so the function is able to return proper error codes. Signed-off-by: Daniel Wagner <[email protected]>
MI doesn't access the complete identify data struct and thus only transfers a port of it. Add a generic version of this function. Signed-off-by: Daniel Wagner <[email protected]>
Use the generic nvme_identify{_partial} interface and drop
the MI specific version.
Signed-off-by: Daniel Wagner <[email protected]>
Since the warning message output by kernel-doc-check. Signed-off-by: Tokunori Ikegami <[email protected]>
Signed-off-by: Daniel Wagner <[email protected]>
The next change will add a static inline implementation of nvme_get_log which uses nvme_get_log_page. Thus move the nvme_get_log_page function up so it can be used directly. Signed-off-by: Daniel Wagner <[email protected]>
Refactor nvme_get_log and nvme_get_log_page to use use struct nvme_passthru_cmd directly and drop the struct nvme_get_log_args. The struct args have are ABI thus it's hard to change it after a release. The nvme_passthru_cmd buffer is a generic buffer and it's possible to use the setters directly on the buffer in an inline function. Besides a reduced API to maintain it also generates better code and avoids one call less when issue a command. [ikegami.t: update nvme_uring_cmd_amdin_passthru_async] Signed-off-by: Tokunori Ikegami <[email protected]> Signed-off-by: Daniel Wagner <[email protected]>
The compiler warns with
../test/ioctl/logs.c: In function ‘test_get_log_reachability_groups’:
../test/ioctl/logs.c:725:28: warning: enum constant in boolean context [-Wint-in-bool-context]
725 | (!!TEST_LSP << 8) | (!!TEST_RAE << 15) |
| ^
../test/ioctl/logs.c:733:70: warning: enum constant in boolean context [-Wint-in-bool-context]
733 | err = nvme_get_log_reachability_groups(test_link, TEST_RAE, !!TEST_LSP,
| ^
Update the tests so the types are correctly handled. It also addresses
the problem with the wrong lsp value testing.
Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_format_nvm_args. Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_ns_mgmt_args. Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_ns_attach_args. Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_fw_download_args. Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_fw_commit_args. Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_security_send_args. Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_set_features_args. Signed-off-by: Dennis Maisenbacher <[email protected]> [wagi: remove libnvme.map entries reorder fid and sv argument] Signed-off-by: Daniel Wagner <[email protected]> nvme_set_features: fix argument order Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_dsm_args. Signed-off-by: Tokunori Ikegami <[email protected]> [wagi: reorder arguments] Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_io_args. Signed-off-by: Tokunori Ikegami <[email protected]> ioctl: reorder nsid argument Also order the pointer and its length member go to the end of the list. Signed-off-by: Tokunori Ikegami <[email protected]>
Drop struct nvme_lockdown_args. Signed-off-by: Tokunori Ikegami <[email protected]> [wagi: remove libnvme.map entry] Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_capacity_mgmt_args. Signed-off-by: Tokunori Ikegami <[email protected]> [wagi: remove libnvme.map entry reordered/renamed arguments] Signed-off-by: Daniel Wagner <[email protected]>
Drop struct nvme_get_features_args. Signed-off-by: Dennis Maisenbacher <[email protected]> [wagi: reordered/renamed arguments] Signed-off-by: Daniel Wagner <[email protected]>
There is no user left of this type in ioctl.c. Remove it. Signed-off-by: Daniel Wagner <[email protected]>
This is to maintain nvme_get_log_partial easily. Signed-off-by: Tokunori Ikegami <[email protected]>
Since the following warnings output. $ ./scripts/kernel-doc-check test/zns.c test/zns.c:3: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst test/zns.c:10: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst Signed-off-by: Tokunori Ikegami <[email protected]>
examples/mi-mctp.c:3: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst Signed-off-by: Tokunori Ikegami <[email protected]>
Since checked by kernel-doc-check then the warning message output. Signed-off-by: Tokunori Ikegami <[email protected]>
To fix the warning message below. test/mi.c:3: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst Signed-off-by: Tokunori Ikegami <[email protected]>
6291d07 to
3dd78e5
Compare
MaisenbacherD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter deletion looks good :)
We just need to touch this section one more time with #23
src/nvme/ioctl.h
Outdated
| * @cev: Command Extension Value | ||
| * @dspec: Directive Specific | ||
| * @lbatm: Logical block application tag mask | ||
| * @control: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description got lost here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fixed the error. Thank you.
0f04d6a to
660b33e
Compare
Since incorrectly copied the nvme_copy parameters. Signed-off-by: Tokunori Ikegami <[email protected]>
3dd78e5 to
75fff83
Compare
|
Just fixed the review comment error. So should we rebase the PR changes with the latest libnvme2 branch? (I tried to do it but many conflict errors caused so let me confirm it before doing to resolve to make sure.) |
|
I would say this PR could be rebased onto libnvme2 as I am not aware of any other ongoing work in the same hunk. Please correct me if I am wrong @igaw |
|
I'll cherry-pick this patch. No need for rebasing |
|
I see. Thank you. |
| * @nlb: Number of logical blocks | ||
| * @control: Upper 16 bits of cdw12 | ||
| * @cev: Command Extension Value | ||
| * @dspec: Directive Specific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which verison of the spec are you looking at? In the latest version cev and dspec are also valid for zone append.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay since actually the API nvme_zns_append() does not use the parameters so should we change the API to use the parameters correctly also?
| * @control: Upper 16 bits of cdw12 | ||
| * @lbat: Logical block application tag | ||
| * @lbatm: Logical block application tag mask | ||
| * @ilbrt_u64: Initial logical block reference tag - 8 byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @MaisenbacherD figured out, we should use nvme_set_var_size_tags here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As same actually the paramters are used the API then looks as the API needed to change to use nvme_set_var_size_tags as mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ikegami-t Sorry I was not aware that I had to touch the nvme_zns_append parameters again when I did the reviews on your commits for the same function.
I just posted the PR #38
I am happy to rebase my changes and fix up the merge conflicts when your commit get merged before mine :)
e10ee25 to
1ee86ed
Compare
|
I've merged PR#38 which should address the issue reported there. Thanks nonetheless! |
Since incorrectly copied the nvme_copy parameters.